Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/sc 2663/wrap amm quote requests in api #27

Merged
merged 3 commits into from
May 23, 2024

Conversation

jackmellis
Copy link
Collaborator

@jackmellis jackmellis commented May 22, 2024

feat: ERC20 quotes can now be obtained via the nftx api

this is a huge refactor and rewrite of the pricing api, covering @nftx/api, @nftx/core, and @nftx/trade

the individual quoting methods (fetchVaultBuyPrice etc) have been removed in favour of two methods: fetchQuote, and fetchPrice
fetchPrice returns an off-chain price, fetchQuote returns an on-chain quote with executable data
the different types of pricing calls can now be chosen via the type parameter, which can be buy/sell/swap/mint/redeem and the new type: erc20

BREAKING CHANGE: price methods have been removed in favour of fetchPrice and fetchQuote
BREAKING CHANGE: tradeErc20 method now takes a quote object (obtained from fetchQuote) instead of a price object (from fetchTokenBuyPrice)
BREAKING CHANGE: fetchTokenBuyPrice has been replaced with fetchAmmQuote - though fetchQuote/fetchPrice should be preferred


refactor: move fetchEthPrice / fetchSpread / fetchSpotPrice to @nftx/api

these methods were previously in @nftx/trade and interacted directly with the AMM router
as we now have an API layer for requesting ERC20 quotes, these methods have been moved to @nftx/api

BREAKING CHANGE: these methods are no longer available in @nftx/trade


feat(@nftx/trade): add fulfill function

this function takes any quote from fetchQuote and fulfills it, regardless of whether it's a sell/buy, a mint/redeem, or an ERC20 quote


Also worth noting that the changes to @nftx/api will require a change to the nftx-api deployment - which in turn relies on this PR so will need to follow up immediately after releasing!

this is a huge refactor and rewrite of the pricing api, covering @nftx/api, @nftx/core, and @nftx/trade

the individual quoting methods (fetchVaultBuyPrice etc) have been removed in favour of two methods: fetchQuote, and fetchPrice
fetchPrice returns an off-chain price, fetchQuote returns an on-chain quote with executable data
the different types of pricing calls can now be chosen via the `type` parameter, which can be buy/sell/swap/mint/redeem and the new type: erc20

BREAKING CHANGE: price methods have been removed in favour of fetchPrice and fetchQuote
BREAKING CHANGE: tradeErc20 method now takes a quote object (obtained from fetchQuote) instead of a price object (from fetchTokenBuyPrice)
BREAKING CHANGE: fetchTokenBuyPrice has been replaced with fetchAmmQuote - though fetchQuote/fetchPrice should be preferred
these methods were previously in @nftx/trade and interacted directly with the AMM router
as we now have an API layer for requesting ERC20 quotes, these methods have been moved to @nftx/api

BREAKING CHANGE: these methods are no longer available in @nftx/trade
this function takes any quote from fetchQuote and fulfills it, regardless of whether it's a sell/buy, a mint/redeem, or an ERC20 quote
@jackmellis jackmellis force-pushed the feature/sc-2663/wrap-amm-quote-requests-in-api branch from 0c8370d to 4c11b2e Compare May 22, 2024 12:53
@jackmellis jackmellis marked this pull request as ready for review May 22, 2024 12:54
function fetchQuote(args: QuoteArgs): Promise<MarketplaceQuote>;
function fetchQuote(args: PriceArgs | QuoteArgs) {
/** Returns an on-chain quote for a transaction. The response object can be passed into @nftx/trade's fulfill method to execute the quote */
function fetchQuote(args: BuyArgs & PriceArgs): Promise<MarketplacePrice>;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it easier/more readable just to have an interface with all the props?

Copy link
Collaborator Author

@jackmellis jackmellis May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it this way so ts can tell you which props you actually need to provide for each type of txn, otherwise it's a whole mess of optional props and it'd be difficult to know which things you need to pass and when

Copy link

@0xNumlock 0xNumlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, lgtm.

@jackmellis jackmellis merged commit 7ec4899 into main May 23, 2024
1 check passed
@jackmellis jackmellis deleted the feature/sc-2663/wrap-amm-quote-requests-in-api branch May 23, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants